-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Combobox component to Toucan Core & Toucan Form #200
Conversation
🦋 Changeset detectedLatest commit: 212cd77 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Preview URLsEnv: preview |
1760d32
to
bc2e3bf
Compare
bc2e3bf
to
e9c4bd6
Compare
e9c4bd6
to
958e3b6
Compare
958e3b6
to
3b708a0
Compare
3b708a0
to
fd79fef
Compare
fd79fef
to
c375b01
Compare
c375b01
to
807b41f
Compare
This reverts commit 760601b.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, with some suggestions and remarks. I'm totally fine with either dismissing them if you disagree, or tackling them in future iterations, as I don't want to block this big PR any further...
|
||
## Popover z-index | ||
|
||
A CSS class to add to this component's content container. Commonly used to specify a `z-index`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commonly used to specify a
z-index
.
Do users actually have to do this? This is no headless component, instead we do apply everything that's needed to make it look right. So shouldn't this "just work"(TM)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More info over on #200 (comment), but hopefully this is temporary and we can remove it soon™. We just need to figure out where/if some of the CSS variables we have should be defined!
@@ -135,6 +135,7 @@ | |||
"ember-browser-services": "^4.0.4", | |||
"ember-modifier": "^4.1.0", | |||
"ember-resources": "^6.0.0", | |||
"ember-velcro": "^2.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have to add it here? The addon already has it as a dependency. so that should be enough?
{{on "click" this.onClick}} | ||
{{! template-lint-disable no-pointer-down-event-binding }} | ||
{{on "mousedown" this.onMousedown}} | ||
{{on "mouseover" @onMouseover}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need explicit mouseover
support here? We have ...attributes
, so any event listener can be added? The @onClick
support seems to be needed for event.preventDefault();
to be baked in. But this doesn't have it, so users could just apply {{on "mouseover"}}
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! Yeah, I think we probably could.
This gets used in combobox.hbs
where we yield back Option
. We do the following:
onMouseover=(fn this.onOptionMouseover index)
Is there a way to make this a modifier rather than a component argument when we are yielding back?
|
||
this.closePopover(); | ||
|
||
// This shouldn't be possible, but to satisfy TS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: If it's not possible to be in that state, then we can use assert()
, which makes TS happy and does not add runtime code (in prod).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! Pushed as part of commit 4f2d139
test-app/config/ember-try.js
Outdated
}, | ||
}, | ||
}), | ||
// @todo Temporarily disabling embroider optimized tests due to https://github.com/CrowdStrike/ember-toucan-core/issues/210 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: would have been enough to just not comment out the embroider scenarios in ci.yml
as you did, and leave this as-is...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with 0c8f958
assert.dom('[data-lock-icon]').exists(); | ||
}); | ||
|
||
test('it spreads attributes to the underlying textarea', async function (assert) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testarea? 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Turns out a lot of copy pasta (ac3991c)
/** | ||
* The function called when a user types into the combobox textbox, typically used to write custom filtering logic. | ||
*/ | ||
onFilter?: (input: string) => OPTION[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw we are calling this with ´await`, so should be this?
onFilter?: (input: string) => OPTION[]; | |
onFilter?: (input: string) => OPTION[] | Promise<OPTION[]>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, I backed out of it being a promise for now due as the current implementation is not promise based. Let me revert the await
! We can add that support back if we find out folks need it! d036a43
} | ||
|
||
if (onFilter) { | ||
filteredOptions = await onFilter(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also pass the raw options here, so the function can be simple and not need to have a of closure to have access to the options? Like
filteredOptions = await onFilter(value); | |
filteredOptions = await onFilter(optionsArgument, value); |
Another option (pun intended) would be to make this function a higher order function that returns the filter function, so you could do:
filteredOptions = await onFilter(value); | |
const filterFn = onFilter(value); | |
filteredOptions = optionsArgument.filter(filterFn); |
We lose the ability for async though. But it would have the benefit that you can only return options that are actually members of @options
. Which would be weird if that wasn't the case, and likely cause problems (e.g. when doing ===
checks). In the suggestion above such a mistake would be less likely, and more likely in the original implementation here.
I don't have strong opinions on this though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah for sure, I could see this being helpful. I'm going to go ahead and merge as-is if that's okay due to our protection with changesets and am going to chat up @clintcs on Monday about this!
} | ||
``` | ||
|
||
## onFilter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be marked as optional, right? Maybe also point out what the default filtering logic does, so it's clear you don't need this if the defaults work as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wrote some more as part of 2b74de0, thank you!
Co-authored-by: Simon Ihmig <[email protected]>
Co-authored-by: simonihmig <[email protected]>
Co-authored-by: simonihmig <[email protected]>
Co-authored-by: simonihmig <[email protected]>
Co-authored-by: simonihmig <[email protected]>
Co-authored-by: simonihmig <[email protected]>
Co-authored-by: simonihmig <[email protected]>
Co-authored-by: simonihmig <[email protected]>
🚀 Description
This PR builds on top of the work @clintcs had done on the select component. It was decided to make this a
combobox
instead. I apologize how huge this PR is, there was already a lot going on and I wanted to make sure everything worked end-to-end.API Considerations
@onChange
to@onSelect
@optionKey
At the start of this component, it was thought that
@optionKey
was overkill and a component API smell; however, it turns out it has some advantages. You can see where I changed from filterBy to optionKey+onFilter in this commit.Here is how
@optionKey
makes our lives easier:By using
@optionKey
, we can set thevalue
of the input toselected[optionKey]
and useoptionKey
in the default search scenario.Checklist of functionality / features post-handoff from Clinton
aria-expanded
(a11y)role="combobox"
(a11y)aria-activedescendant
(a11y)@optionKey
🔬 How to Test
I've written a ton of tests on the usability and logic for how this should all work from a UI/UX perspective based on our chats with design. Please go check out those tests and play with the Combobox preview URLs!
To test via preview URLs
Colors
select controlb
Colors
select controlb
To test things locally
(optional)
Testing locally is optional, as there are a ton of tests written for the functionality and the preview URL should suffice for testing usability; however, if you want to:
📸 Images/Videos of Functionality
Screen.Recording.2023-07-07.at.10.09.29.AM.mov
Screen.Recording.2023-07-07.at.10.13.14.AM.mov
Screen.Recording.2023-07-07.at.10.14.16.AM.mov